Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: enable minio webhook #256

Merged
merged 1 commit into from
Oct 24, 2024
Merged

feat: enable minio webhook #256

merged 1 commit into from
Oct 24, 2024
+14 −0

Conversation

ambersun1234
Copy link
Contributor

@ambersun1234 ambersun1234 commented Oct 16, 2024 •
edited by ai-themis bot
Loading

PR Type

Enhancement

Description

  • Introduced a ConfigMap for MinIO webhook settings to enable webhook notifications.
  • Added a new parameter extraEnvVarsCM in the MinIO configuration to reference the ConfigMap.
  • Updated the MinIO configuration documentation for clarity.

Changes walkthrough 📝

Relevant files
Enhancement
agh-minio-cm.yml
Add ConfigMap for MinIO webhook configuration                       

charts/agh3/templates/minio/agh-minio-cm.yml

  • Added a ConfigMap for MinIO webhook configuration.
  • ConfigMap includes settings for enabling the webhook and specifying
    the endpoint.
    		•  +9/-0     
    values.yaml
    Update MinIO values with extraEnvVarsCM parameter               

    charts/agh3/values.yaml

  • Introduced a new parameter extraEnvVarsCM for MinIO configuration.
  • Updated documentation to reflect the new configuration option.
    		•  +3/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @ambersun1234 ambersun1234 self-assigned this Oct 16, 2024
    @ai-themis ai-themis bot added the enhancement New feature or request label Oct 16, 2024
    @ai-themis
    Copy link

    ai-themis bot commented Oct 16, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Smell
    The MINIO_NOTIFY_WEBHOOK_ENDPOINT_PRIMARY is constructed using a hardcoded URL pattern. Consider making this more configurable to accommodate different environments or URL structures.

    @ai-themis
    Copy link

    ai-themis bot commented Oct 16, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure the endpoint value is properly quoted to avoid parsing issues

    Ensure that the MINIO_NOTIFY_WEBHOOK_ENDPOINT_PRIMARY value is properly quoted to
    prevent any potential parsing issues with special characters or spaces.

    charts/agh3/templates/minio/agh-minio-cm.yml [8]

    -MINIO_NOTIFY_WEBHOOK_ENDPOINT_PRIMARY: {{ printf "http://captain.%s.svc.cluster.local:8080/internal/webhook/minio" .Release.Namespace | quote }}
+MINIO_NOTIFY_WEBHOOK_ENDPOINT_PRIMARY: "{{ printf "http://captain.%s.svc.cluster.local:8080/internal/webhook/minio" .Release.Namespace | quote }}"
    Suggestion importance[1-10]: 8

    Why: Quoting the endpoint value is a good practice to avoid parsing issues, especially with special characters. However, the current implementation already uses the quote function, which mitigates the risk, hence the score is not higher.

    		8

    knowlet
    knowlet requested changes Oct 24, 2024
    View reviewed changes
    Copy link
    Contributor

    @knowlet knowlet left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Plz use helm template to verify.

    charts/agh3/templates/minio/agh-minio-cm.yml Outdated Show resolved Hide resolved
    @ambersun1234 ambersun1234 requested a review from knowlet October 24, 2024 07:50
    @knowlet knowlet merged commit 24f32c9 into main Oct 24, 2024
    1 check passed
    @knowlet knowlet deleted the feat/minio-webhook branch October 24, 2024 07:54
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @knowlet knowlet knowlet approved these changes

    @Aries0d0f Aries0d0f Awaiting requested review from Aries0d0f

    Assignees

    @ambersun1234 ambersun1234

    Labels
    enhancement New feature or request Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @ambersun1234 @knowlet